Fixing issue with single quotes being lost from input#27
Conversation
Summary of ChangesHello @j-wang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug where single quotes in input data were being lost when processed by AppleScript/JXA commands, specifically impacting Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where single quotes in input were being lost due to improper shell escaping. While the fix works, I've provided a suggestion to refactor the code to use child_process.execFile instead of child_process.exec. This is a more robust and secure approach as it avoids shell interpretation entirely, removing the need for manual string escaping. This would prevent this class of bug from recurring. Additionally, it would be beneficial to add a regression test for this fix to ensure it doesn't happen again.
| export const executeJxa = <T>(script: string): Promise<T> => { | ||
| return new Promise((resolve, reject) => { | ||
| const command = `osascript -l JavaScript -e '${script.replace(/'/g, "''")}'`; | ||
| const command = `osascript -l JavaScript -e '${script.replace(/'/g, "'\\''")}'`; |
There was a problem hiding this comment.
While this fix for escaping single quotes is correct, constructing shell commands by interpolating strings is fragile and can be a security risk (shell injection). A more robust and secure approach is to use child_process.execFile, which passes arguments to the command directly, bypassing the shell. This eliminates the need for manual escaping.
I recommend refactoring this function to use execFile. You would need to change the import on line 1 from exec to execFile, and then modify the execution logic as follows:
// in executeJxa function body
execFile("osascript", ["-l", "JavaScript", "-e", script], (error, stdout, stderr) => {
// ... rest of the callback remains the same
});This change makes the code safer, simpler, and removes a whole class of potential escaping-related bugs.
There was a problem hiding this comment.
Sure. I agree with this. I suppose it depends on whether @dvcrn is more comfortable with this vs. literally two extra characters, but I'll make the change and update.
|
Retested, and it's all working from Claude calling it without issues. |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of single quotes being lost from input by refactoring the external command execution. The transition from child_process.exec to child_process.execFile is a crucial improvement, as it enhances security by preventing potential shell injection vulnerabilities and correctly handles script arguments without relying on manual string escaping. This change represents a more robust and recommended practice for executing external processes.
| return new Promise((resolve, reject) => { | ||
| const command = `osascript -l JavaScript -e '${script.replace(/'/g, "''")}'`; | ||
| exec(command, (error, stdout, stderr) => { | ||
| execFile("osascript", ["-l", "JavaScript", "-e", script], (error, stdout, stderr) => { |
There was a problem hiding this comment.
The change from exec to execFile with an array of arguments is a significant security enhancement. The previous method of constructing a command string and manually escaping characters was inherently vulnerable to shell injection if not meticulously handled for all special characters. execFile bypasses the shell's interpretation of arguments, making the execution much safer and more reliable, directly addressing the issue of lost single quotes.
|
Will try to get this released today, thanks for the PR! ❤️ |
|
Released in 1.7.1 |
2 participants
This impacts create_record and update_record_content at minimum. Fixes #26.
I've traced the cause, which is a fairly simple fix in
execute.ts's escape pattern. I've tested it on records myself in Claude, which is also working.All tests and linting pass—which shouldn't be surprising, given how small of a change this is.